Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_ast_lowering/src/format.rs

cc @m-ou-se

r? @spastorino

rustbot has assigned @spastorino.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot author

@rustbot ready

Nominating as per #145739 (comment)

It'd be worth adding a test for the drop behavior.

Given that this makes more sense for the language, along with the clean crater results and the intuition that it'd be surprising if anything actually leaned on this, I propose:

@rfcbot fcp merge lang

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

I don't think we should do this. It will make the generated code for println!("{x} {x}"); less efficient, as it will get two separate arguments instead of one.

I don't want to end up in a situation where it would make sense for Clippy to suggest something like:

warning: using the same placeholder multiple times is inefficient as of Rust 1.94.0
 --> src/main.rs:3:5
  |
3 |     println!("{x} {x}");
  |     ^^^^^^^^^^^^^^^^^^^
  |
help: change this to
  |
3 -     println!("{x} {x}");
3 +     println!("{x} {x}", x = x);
  |

Adding , x = x shouldn't make a difference. If adding that makes the resulting code more efficient, I strongly feel like we've done something wrong.

@rust-rfcbot concern equivalence

Yes, I'm stating that throwing out this equivalence because of wonkiness when using things that are often specifically highlighted as notoriously bad ideas is a bad idea.

Could we get what we want as a lang matter and keep most of the optimization benefit by doing the optimization only when it's guaranteed to not be observable, e.g. when values of the type don't need to be dropped and can't be interior mutable? Is that feasible?

Could we get what we want as a lang matter and keep most of the optimization benefit by doing the optimization only when it's guaranteed to not be observable, e.g. when values of the type don't need to be dropped and can't be interior mutable? Is that feasible?

I don't think there's much we can do about pathological Debug or Display impls that make it observable when something is formatted. In general, that doesn't even require interior mutability. Rust's type system just isn't equipped to reason about it; we don't have pure functions. I think the best we could get is to treat derived impls as trusted if fields' impls are trusted and do a pass where we mess with how format_args! expands based on whether we can prove formatting doesn't execute any untrusted code. But that leaves a lot of uses unoptimized and it's overengineered if all it's solving is this.

If we're only concerned with how many drop-sensitive temporaries are created, this is much easier to solve. We could optimize in the most common case (multiple mentions of local variables in a formatting string) without even needing type information when expanding format_args!, since taking a reference to a variable won't create a temporary; it's just named constants and nullary constructors where that can be an issue.

I'm just gonna initiate a perf run to just see if there is any observable regressions under the current changes (though, recognizing a negative signal here is not super meaningful).

@bors try @rust-timer queue

@rfcbot fcp cancel

@traviscross proposal cancelled.

@rfcbot fcp merge lang,libs-api

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @nikomatsakis
• @scottmcm
• @the8472
• @tmandry
• @traviscross

Concerns:

• equivalence (Do not deduplicate captured args while expanding format_args! #149926 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

I shared @m-ou-se's concern from above:

I don't think we should do this. It will make the generated code for println!("{x} {x}"); less efficient, as it will get two separate arguments instead of one.

I don't want to end up in a situation where it would make sense for Clippy to suggest something like:

warning: using the same placeholder multiple times is inefficient as of Rust 1.94.0
 --> src/main.rs:3:5
  |
3 |     println!("{x} {x}");
  |     ^^^^^^^^^^^^^^^^^^^
  |
help: change this to
  |
3 -     println!("{x} {x}");
3 +     println!("{x} {x}", x = x);
  |

Adding , x = x shouldn't make a difference. If adding that makes the resulting code more efficient, I strongly feel like we've done something wrong.

@rfcbot concern equivalence

@BurntSushi, @the8472, perhaps you could speak to the suggestion that we can optimize things at other layers, e.g. as @dianne described:

If we're only concerned with how many drop-sensitive temporaries are created, this is much easier to solve. We could optimize in the most common case (multiple mentions of local variables in a formatting string) without even needing type information when expanding format_args!, since taking a reference to a variable won't create a temporary; it's just named constants and nullary constructors where that can be an issue.

Also, could you both speak to whether, for these optimization reasons, you'd similarly expect that we would desugar format_args!("{x.f} {x.f}") as format_args!("{0} {0}", x.f)? For format_args!("{x.y} {x.y.z}"), what desugaring would you expect?

☀️ Try build successful (CI)
Build commit: 0282858 (0282858611f50e7d4c722e4a5295fc552f7236a1, parent: 625b63f9e148d511e187c71e5f70643ee62c77fb)

It's more that my expectation is that these ought to be pure and we should optimize accordingly. And I haven't seen anyone making the argument that real-world code actually expects or relies on side-effects happening in print expressions, so why should we go out of our way to make code less efficient to enable something nobody needs?

We could optimize in the most common case (multiple mentions of local variables in a formatting string) without even needing type information when expanding format_args!, since taking a reference to a variable won't create a temporary

I guess that'd be fine, yeah.

We already enabled that -- you can have drop glue in consts, and you can print consts. So the question is, do we have nice regular behavior or do we add weird exceptions to the semantics of the language?

Arguably macros are not semantics of the language, but arbitrary stuff a macro does to rewrite your code, no?

Insofar as format_args is a builtin macro, its behavior is part of the language.

Finished benchmarking commit (0282858): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Bootstrap: 474.946s -> 472.809s (-0.45%)
Artifact size: 383.21 MiB -> 383.22 MiB (0.00%)

First, I think @m-ou-se's concern above is worth considering. It's not uncommon to use a variable multiple times in a format string, and it would be unfortunate if we were forced to include the same reference multiple times, since you would never do that with hand-written assembly.

With that said, @dianne's suggestion resolves the concern for me. We can optimize away the common case of taking a reference to the same variable multiple times.

What pushes me over the edge toward thinking we should do this is not the const example by itself; it's an odd edge case for sure. What I find more motivating is to think about what should happen in a generalization of the current behavior. For example, if we ever accept expressions with function calls, I would not expect println!("{foo()}, {foo()}") to deduplicate the call to foo. I would also say that the LoudDrop example is closer to this and therefore more motivating than the const example.

Regarding the wording in the RFC, it's useful as a fallback option, but the only reason I would find it persuasive is if we had discussed this case explicitly and decided to go with the current behavior. We could still change our minds with an FCP, but knowing that would be important context.

With all that said,

@rfcbot reviewed